Add useful functions for external use of WASI filesystem #595
Conversation
lib/wasi/src/state.rs
Outdated
| pub fn close(self) {} | ||
| /// This trait relies on your file closing when it goes out of scope via `Drop` | ||
| /// it does not require `Drop` because `std::fs::File` does not implement it | ||
| /// (TODO: figure out how that makes sense, because it clearly does) |
There was a problem hiding this comment.
std::fs::File does not implement Drop, because it contains a std::sys::fs::File, which does not implement Drop as it contains a std::sys::fd::FileDesc, which implements Drop.
aka, only the innermost item needs to implement Drop, rust figures out the rest on it's own?
There was a problem hiding this comment.
Thanks! I was reading through the code, but didn't get that far -- that's good to know
There was a problem hiding this comment.
so, yeah, apparently we should (almost?) never require an impl of Drop, because of the above.
|
|
||
| #[derive(Debug)] | ||
| pub struct Stdout(std::io::Stdout); | ||
| impl Read for Stdout { |
There was a problem hiding this comment.
I feel like a 'nicer' way of doing this is to essentially encapsulate Read/Write/Seek in our trait, and provide default generic implementations if the type implements Read/Write/Seek.
(this is probably not nicer in many other ways, so meh.)
There was a problem hiding this comment.
Hmm, yeah, I see what you mean. I think this is fine because while it is garbage code, we only need to write it once -- and I'll refactor the state.rs file to hide things like this because we mostly never need to see it -- and it lets us use all code working with the standard traits including third party libraries which could save us a lot of time in the future.
There was a problem hiding this comment.
I mean, so does this approach. The issue (either way, really) is that if a third party library is missing any one of these three, you have to wrap it in a newtype and create dummy implementations. At least with the 'wrapper trait' approach, you aren't implementing, say, Seek on something that can't actually support Seek. shrug
only solution I can think of that doesn't require boilerplate is probably 'wrapper trait' + specialization, and even then a) i don't think we want to do that and b) it doesn't appear to work like it does in my head so...
|
I will note that currently this PR by itself does not expose any extra functionality, as WasiFs/WasiState and such is still not exported at the crate top-level. EDIT: now it does. |
|
@kitlith specifically, /// Get WasiState from a Ctx
pub unsafe fn get_wasi_state(ctx: &mut Ctx) -> &mut WasiState {
&mut *(ctx.data as *mut WasiState)
}Presumably in the future we can also provide higher level APIs that don't require using the raw WASI types which I also exported in this PR. I wrapped the error type for the public facing APIs and maybe we can do the same with permissions and capabilities as well. The fd capabilities should map very closely to what |
|
bors r+ |
595: Add useful functions for external use of WASI filesystem r=MarkMcCaskey a=MarkMcCaskey part of #583 Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: Mark McCaskey <[email protected]>
|
bors r- |
Canceled |
|
I updated the plugin example to use the new logging feature. It relies on the guest calling into the host. I think we will probably definitely need the builder pattern, too. Because relying on the guest to do that is less than ideal. There's also the possibility of solving this problem with Wasm transformations (the host performing arbitrary modifications to the Guest's Wasm before running) but I don't think that's something we support very well right now |
|
bors r+ |
595: Add useful functions for external use of WASI filesystem r=MarkMcCaskey a=MarkMcCaskey part of #583 Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: Mark McCaskey <[email protected]>
|
I don't really see why the guest needs to call into the host, when there's wasmer_runtime::Instance::context_mut? I may be missing something. And I don't think get_wasi_state should have been changed into a safe function, as it is only sound if the Ctx came from your WASI implementation, right? We (currently) must rely on the user to know this... |
|
@kitlith Wow, I did not know about that at all, but it makes sense that that would exist. That would actually be useful in a personal project I'm making with Wasmer. Thanks! I'll update the example. Hmm, that's a fair point. So the reason why I made it safe was because in our code we're casting a |
part of #583